Skip to content

Conversation

jdesrosiers
Copy link
Member

Resolves #1231
Replaces #1600

This is a full rewrite of the Security Considerations sections of the Core spec. It retains most of the original content plus a lot more. The only thing I left out from the original is the part about $comment, which I don't think makes sense. If there's an argument for keeping it, I can add it back in.

This new Security Consideration section is inspired by the guidelines and examples in RFC 3552 - Guidelines for Writing RFC Text on Security Considerations. Some principles I'm trying to follow are,

  • This section should not make SHOULD/MUST requirements. Instead, it should state the implications of the requirements made elsewhere in the document.
  • This section may discuss threats that are mitigated by spec requirements, but also makes recommendations for mitigating threats that aren't specifically addressed by the spec.
  • The audience of the spec is implementers. Schema authors should only be referred to in terms of how implementations should handle their schemas.

Something worth mentioning is that this PR advises not to use file: URIs in $id. However, we use file URIs in a couple tests in the official test suite. We should consider changing, removing, or moving those tests to optional.

Comment on lines 2079 to 2080
recursive schemas create loops, but implementations are advised to detect and
break these cycles when they are encountered.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier in the spec, implementations are required to halt when encountering infinite cycles.

How would one break such a cycle?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't intend to imply that one could somehow continue evaluating the schema. I'll reword to make it more clear that it should stop evaluation when it detects a cycle.

Copy link
Member

@karenetheridge karenetheridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one comment, otherwise 👍🏼

Comment on lines 2090 to 2093
Implementations should consider rejecting schemas that have identifiers
(including embedded schema identifiers) that conflict with registered schemas
and should apply consistent URI normalization and comparison logic to detect and
prevent conflicts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would we not want to allow that? I think we should upgrade this to a MUST.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think (not in front of a computer currently) that normalization is a requirement of the sections on reference resolution.

That said, I wouldn't be opposed to being more strict around preventing overwrites.

Would a MUST-level requirement be appropriate here, or would it fit more in one of the main body sections?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all ...

Would a MUST-level requirement be appropriate here, or would it fit more in one of the main body sections?

Right. The Security Considerations section should be an analysis of the security considerations both mitigated and not mitigated by requirements as defined in the specification. It should not make normative statements, but can reference parts of the spec that do.

When would we not want to allow that? I think we should upgrade this to a MUST.

There are two "shoulds" in this sentence, so I'm not sure which one (or both) are being referred to.

Implementations should consider rejecting schemas that have identifiers (including embedded schema identifiers) that conflict with registered schemas

A reasonable alternative could be to ignore the schema and use the registered one instead (perhaps with a warning). A scenario where I think this might make sense is with a bundled schema that includes a common schema because it might be used in a variety of implementations and doesn't know if the schema will be registered or not. If the implementation doesn't have that schema registered, then it's available in the bundle. If the implementation does have it registered, it should use the registered version. I think there's a variety of different situations and use cases where not rejecting the schema could make sense. I'd like implementations to be able to provide users with the ability to choose how this should be handled. But, suggesting that they should be rejected is, I think, I good "secure by default" behavior.

and should apply consistent URI normalization and comparison logic to detect and
prevent conflicts.

Looking into this, I just noticed the following from RFC 3987

Applications using IRIs as identity tokens with no relationship to a protocol MUST use the Simple String Comparison (see section 5.3.1). All other applications MUST select one of the comparison practices from the Comparison Ladder (see section 5.3

JSON Schema describes our use of IRIs as identity tokens, not locators, which means implementations should limit themselves to Simple String Comparison. That's not what our spec currently says. It says to use the normalization procedure defined in 5.3, which isn't quite correct either. We'd need to be more specific about which comparisons to use. For example, we wouldn't require scheme-based or protocol-based normalization because it would be too onerous a requirement for implementers to implement scheme-based requirements for any scheme they might encounter.

From a security perspective, it doesn't matter what normalization is performed, only that it's consistent. Maybe that's the update we should make for now and discuss what comparison/normalization should be required in the spec in a separate issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can think of a clear usecase for this.
It may be that implementations have chosen poor architecture and only allow a single instance of the implementation.
Say I have a JSON Schema, and I want to evaluate the outcome if I change a part of one of the referenced schemas? If it was already registered with an ID, and I want to override that, I should be able to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that comes down to an implementation option to allow it. The default behavior should be to reject an overwrite attempt.

Copy link
Member

@Relequestual Relequestual left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Thanks for your work on this. Sorry for the delay in reviewing.

@jdesrosiers
Copy link
Member Author

It seems like it might be incorrect for us to suggest that implementers to normalize identifiers (see #1618 (comment))

For now, I made a slight change to the wording about URI normalization and I suggest we merge now and consider the question of normalization separately. I'll create a follow up issue.

Comment on lines 2150 to 2154
### Vocabulary-Specific Risks

Third-party JSON Schema vocabularies may introduce additional risks.
Implementers are advised to consult the specifications of any extensions they
support and take into account their security considerations as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update this to use "extension" language since we've removed "vocabulary".

@jdesrosiers jdesrosiers merged commit d42d4b0 into json-schema-org:main Sep 1, 2025
4 checks passed
@jdesrosiers jdesrosiers deleted the sec-cons branch September 1, 2025 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security considerations should mention treating URIs as URLs (from $ref and $schema)
4 participants